Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use Long_val for sendfile() parameters to fix file copying in docker #10333

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

tonyfettes
Copy link

I was using dune in docker, and every it copies some large file in a data_only_dirs it reports that sendfile: No such file or directory. I tried digging into the source and I think converting the ocaml integer to 32-bit version will make it impossible to copy any file that is larger than 4 GB. I have tested in a docker, but I cannot really confirm whether it really works on real linux machine (I'm using macOS).

@tonyfettes tonyfettes changed the title fix sendfile fix: use Long_val for sendfile() parameters to fix file copying in docker Mar 29, 2024
@tonyfettes tonyfettes force-pushed the main branch 2 times, most recently from 854b501 to 2acf653 Compare March 29, 2024 13:20
@emillon
Copy link
Collaborator

emillon commented Mar 29, 2024

Thanks, I'll try to test this.

emillon added a commit to emillon/dune that referenced this pull request Mar 29, 2024
See ocaml#10333

Signed-off-by: Etienne Millon <me@emillon.org>
Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked that it fixes the issue with the test in #10334

otherlibs/stdune/src/copyfile_stubs.c Outdated Show resolved Hide resolved
otherlibs/stdune/src/copyfile_stubs.c Show resolved Hide resolved
@emillon emillon requested a review from dra27 March 29, 2024 13:43
@emillon
Copy link
Collaborator

emillon commented Mar 29, 2024

@dra27 does this look ok or are there any considerations for using Long_val here?

@tonyfettes tonyfettes force-pushed the main branch 3 times, most recently from 9333e0f to a7f3665 Compare April 2, 2024 01:37
emillon added a commit to emillon/dune that referenced this pull request Apr 2, 2024
See ocaml#10333

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this pull request Apr 2, 2024
See #10333

Signed-off-by: Etienne Millon <me@emillon.org>
tonyfettes and others added 3 commits April 2, 2024 10:26
Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>
Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>
Signed-off-by: Etienne Millon <me@emillon.org>
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon merged commit c59f7df into ocaml:main Apr 2, 2024
23 of 27 checks passed
@dra27
Copy link
Member

dra27 commented Apr 2, 2024

This will fix files over 4GB on 64-bit systems, but it will still overflow on 32-bit systems. It's a slightly confusingly-named macro, but Long_val converts an OCaml integer value to a value with the same size as a pointer (so a 64-bit integer on 64-bit systems) whereas Int_val converts to a C int (which is always 32bits, as already noted).

The only way to fix this for 32-bit systems is to use an int64 or, with slightly heavier-weight C code, an int63.

I'm not sure how much that level of 32-bit support really matters for Dune, though?

@rgrinberg
Copy link
Member

Yes, there's no need to bother 32 bit support for dune itself.

pmwhite pushed a commit to pmwhite/dune that referenced this pull request Apr 2, 2024
See ocaml#10333

Signed-off-by: Etienne Millon <me@emillon.org>
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Apr 2, 2024
…docker (ocaml#10333)

* fix: sendfile() in docker

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>

* use ssize_t for return value

Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>

* update test

Signed-off-by: Etienne Millon <me@emillon.org>

* Add changelog

Signed-off-by: Etienne Millon <me@emillon.org>

---------

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>
Signed-off-by: Etienne Millon <me@emillon.org>
Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Co-authored-by: Etienne Millon <me@emillon.org>
This was referenced Apr 17, 2024
emillon added a commit to emillon/dune that referenced this pull request Apr 17, 2024
See ocaml#10333

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this pull request Apr 17, 2024
…docker (ocaml#10333)

* fix: sendfile() in docker

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>

* use ssize_t for return value

Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>

* update test

Signed-off-by: Etienne Millon <me@emillon.org>

* Add changelog

Signed-off-by: Etienne Millon <me@emillon.org>

---------

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>
Signed-off-by: Etienne Millon <me@emillon.org>
Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Co-authored-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this pull request Apr 17, 2024
See ocaml#10333

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this pull request Apr 17, 2024
…docker (ocaml#10333)

* fix: sendfile() in docker

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>

* use ssize_t for return value

Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>

* update test

Signed-off-by: Etienne Millon <me@emillon.org>

* Add changelog

Signed-off-by: Etienne Millon <me@emillon.org>

---------

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>
Signed-off-by: Etienne Millon <me@emillon.org>
Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Co-authored-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this pull request Apr 17, 2024
See ocaml#10333

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this pull request Apr 17, 2024
…docker (ocaml#10333)

* fix: sendfile() in docker

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>

* use ssize_t for return value

Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>

* update test

Signed-off-by: Etienne Millon <me@emillon.org>

* Add changelog

Signed-off-by: Etienne Millon <me@emillon.org>

---------

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>
Signed-off-by: Etienne Millon <me@emillon.org>
Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Co-authored-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this pull request Apr 17, 2024
* test: demonstrate overflow with sendfile (#10334)

See #10333

Signed-off-by: Etienne Millon <me@emillon.org>

* fix: use `Long_val` for sendfile() parameters to fix file copying in docker (#10333)

* fix: sendfile() in docker

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>

* use ssize_t for return value

Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>

* update test

Signed-off-by: Etienne Millon <me@emillon.org>

* Add changelog

Signed-off-by: Etienne Millon <me@emillon.org>

---------

Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>
Signed-off-by: Etienne Millon <me@emillon.org>
Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Co-authored-by: Etienne Millon <me@emillon.org>

* fix formatting (#10356)

Signed-off-by: Etienne Millon <me@emillon.org>

* move changelog entry to the right place (#10375)

Signed-off-by: Etienne Millon <me@emillon.org>

---------

Signed-off-by: Etienne Millon <me@emillon.org>
Signed-off-by: Haoxiang Fei <feihaoxiang2014@gmail.com>
Signed-off-by: Haoxiang Fei <tonyfettes@tonyfettes.com>
Co-authored-by: Haoxiang Fei <tonyfettes@tonyfettes.com>
emillon added a commit to emillon/opam-repository that referenced this pull request Apr 17, 2024
CHANGES:

### Fixed

- Fix overflow in sendfile stubs (copy of large files could fail or end with
  truncated files) (ocaml/dune#10333, @tonyfettes)

- Fix crash when a rule with a directory target is disabled with `enabled_if`
  (ocaml/dune#10382, fixes ocaml/dune#10310, @gridbugs)

- melange: remove all restrictions around virtual libraries in Melange. They
  may be used as otherwise in libraries and executables. (ocaml/dune#10412, @anmonteiro)

- spawn: fix compatibility with RHEL7 (ocaml/dune#10428, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants